Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new option - enableMouseSearchInput #1092

Conversation

dkurucz
Copy link
Contributor

@dkurucz dkurucz commented Mar 9, 2020

Summary:
Adds a configurable option to allow mouse input events on any area of the search input, rather than closing the dropdown outright.

Purpose of this feature:
There are instances where users may want to click somewhere in the text they have typed in order to select it and do any number of operations, including but not limited to copying and pasting, deleting certain parts of longer words in order to modify the search string, or to double-click-select an entire word in the search string, or triple-click-select and highlight the entire search string.

Note that this feature is additionally complementary to the other option I have put in another MR, called preserveSelectedText (#1093), which will automatically populate a single-selection (not multiple) vue select, allowing users to click on choices that were previously chosen and improving speed to enter additional or modified text, and improves usability overall.

This feature is flag-enabled and does not break any default functionality.

Please let me know if there is anything needed to improve naming conventions or if I missed any edge cases, thanks!

Doug Kurucz added 3 commits March 9, 2020 18:53
…ent/option-allow-mouseinput-in-search-3.5.1

# Conflicts:
#	src/components/Select.vue
#	tests/unit/Dropdown.spec.js
…rch-3.5.1

# Conflicts:
#	src/components/Select.vue
…ates, also call preventDefault on ignore, fix merge from 3.9.1 updates
@dkurucz
Copy link
Contributor Author

dkurucz commented Mar 19, 2020

@sagalbot any thoughts on getting this in as well as #1093? I ask because my project is currently using a fork that has the functionality in place. Let me know if you need me to do anything else with this work. Thanks!

@sagalbot
Copy link
Owner

sagalbot commented Mar 19, 2020

@dkurucz just had a chance to check it out today. My first instinct is that this should actually be default behavior.

This default behavior feels like a bug to me:

Mar-19-2020 10-20-54

And with enableMouseSearchInput:

Mar-19-2020 10-21-36

I can't think of a use case where enableMouseSearchInput should be false. You can still click the caret to close (although it should probably have cursor: pointer on hover). I think chances of people finding the right prop to enable this behavior is also fairly low.

Do you agree that enableMouseSearchInput should just be default behavior? If so, lets drop the prop and implement it as such, and I'll push it out as a patch release.


This is a bit related to what I was working on in #1067.

@Dares300
Copy link

Dares300 commented Mar 19, 2020 via email

@dkurucz
Copy link
Contributor Author

dkurucz commented Mar 19, 2020

@sagalbot yes I agree with your thoughts on this. I think it might be better to have the opposite as an option, where it would close on click only if the user is expected to pick an option discreetly. There is a case in my project right now where that could be the preferred behavior. I would equate this to a "choose one of the following" type of input, where you also wouldn't even want the user to type to search. However, at this point it would be a nice to have.

If you'd like, I'll take another pass and get it working by default and then put up a proposal for the flag (or maybe a PR if I can get it all in one go).

@dkurucz
Copy link
Contributor Author

dkurucz commented Mar 24, 2020

@sagalbot I have removed the flag and made the behavior default, feel free to re-test, etc, I think this is largely good to go. Let me know if you have other thoughts on this. Thanks!

@dkurucz
Copy link
Contributor Author

dkurucz commented Apr 7, 2020

@sagalbot any thoughts on a timeline to get this feature in? thanks!

@sagalbot
Copy link
Owner

@dkurucz should go out this morning!

@sagalbot sagalbot merged commit e931f23 into sagalbot:master Apr 29, 2020
@github-actions
Copy link

🎉 This PR is included in version 3.10.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MoogyG
Copy link

MoogyG commented Jun 5, 2020

This functionality breaks the use of a router-link in "selected-option" slot like this :

<template v-slot:selected-option="customerScope">
    <router-link class="mr-2" :to="{ name: 'customer', params: { id: customerScope.id } }">
        {{ customerScope.name }}
    </router-link>
</template>

This shouldn't be enabled by default.

Then, I don't see any reference of enableMouseSearchInput in code or documentation.

Finally, there is a mispelling in jest test (enableMouseInputSearch instead of enableMouseSearchInput)

@dkurucz
Copy link
Contributor Author

dkurucz commented Jun 5, 2020

@MoogyG I'll look into the router link issue when I have a moment. In regards to your other concerns, this issue was accepted as default behavior (it was originally coded as an option), as a result no option was included or configured to enable/disable this behavior, so there will be no additions to the documentation about it. The unit test was an oversight, and should be removed since we are no longer testing the option. We will of course want to enhance some other unit test that is in the main line, reusing some of the test, to ensure the default behavior is in fact default, however.

@dkurucz
Copy link
Contributor Author

dkurucz commented Jun 15, 2020

@MoogyG I tested and fixed your above concerns in the following PR: #1211. Feel free to test on your end as well.

@MoogyG
Copy link

MoogyG commented Jun 16, 2020

I just get your PR, build it locally, copy "dist" folder in my node_modules but it's not working on my machine, I still can't click on a router link inside vue-select.

@dkurucz
Copy link
Contributor Author

dkurucz commented Jun 16, 2020

@MoogyG can you verify you also copied the compiled css in addition to the built js into the dist directory of node_modules vue-select? Both are required in that fix. If so, can you send me a screenshot of what you are seeing? Let's also move this discussion to the new PR. Thanks!

@MoogyG
Copy link

MoogyG commented Jun 16, 2020

My bad, I was importing SCSS from src folder, not CSS from dist folder.
Now it works, thanks for your support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants